Skip to content

Fix multiple security vulnerabilities (auth bypass, path traversal, XSS, weak defaults)#7

Open
yourfavDev wants to merge 1 commit into
mainfrom
claude/security-vulnerabilities-kv9Kn
Open

Fix multiple security vulnerabilities (auth bypass, path traversal, XSS, weak defaults)#7
yourfavDev wants to merge 1 commit into
mainfrom
claude/security-vulnerabilities-kv9Kn

Conversation

@yourfavDev
Copy link
Copy Markdown
Owner

Summary

Audit of the codebase surfaced several high-impact issues. Each is addressed in this PR.

1. Unauthenticated data exfiltration (Critical)

GET /{table}/store, GET /{table}/keys, POST /{table}/keys and the SSE GET /{table}/subscribe/{key} endpoint required no authentication. Any anonymous client could:

curl http://node/auth/store

and walk away with every user's SHA256-hashed password for offline cracking, plus the full contents of every table.

Fix: all four endpoints now require a valid JWT and only return rows whose owner == sub claim.

2. Insecure secret fallbacks (Critical)

  • ADMIN_PASSWORD silently defaulted to "admin123".
  • CLUSTER_SECRET defaulted to "default_secret" everywhere.
  • The admin module had a separate get_jwt_secret() that defaulted to "default_jwt_secret".

Combined with the existing X-Internal-Request: <CLUSTER_SECRET> bypass in get_value, put_value_internal and delete_value, a node that booted without configured secrets would happily read/write/delete any record for any caller using the default header.

Fix: main.rs now refuses to boot unless JWT_SECRET, CLUSTER_SECRET and ADMIN_PASSWORD are all set, ≥16 bytes long, and not in the known-placeholder list ("", "default_secret", "please_change", "admin123", ...). admin_login no longer falls back to a hard-coded password and now uses a constant-time comparison. Admin signing/verification uses the strict engine::get_jwt_secret.

3. Path traversal via {table} / {key} (High)

Table names from URL paths are joined into filesystem paths in persistance.rs (Path::new(state.base_dir).join(&t_name)). A request like

PUT /..%2F..%2Ftmp%2Fevil/key/foo

would write outside ./data.

Fix: new is_safe_name() helper rejects empty names, ., .., names containing /, \, or \0, and names longer than 255 bytes. Applied to every handler (get_value, put_value, patch_value, delete_value, put_value_internal, get_table_store, get_all_keys, get_multiple_keys, subscribe_to_key, admin handlers, authentication::access).

4. Plaintext cluster traffic when DYNA_MODE=https (High)

engine.rs, admin.rs and authentication.rs hard-coded format!("http://{}/...") for all internal replication URLs even when TLS was configured, leaking the cluster secret and every replicated value over the wire.

Fix: network::broadcaster::build_url was already TLS-aware; it is now pub and used by every internal call site.

5. Race condition + ownership forgery in user registration (High)

authentication::access did a non-atomic auth_table.get(&username) then auth_table.insert(...), so two concurrent registrations of the same username could race past each other. The internal replication branch also trusted a caller-controlled User: header as the record owner.

Fix: registration is now atomic via DashMap's entry().and_modify(...).or_insert_with(...). The owner is always the path-derived username; the spoofable User: header is gone. Passwords are now salted with the username before SHA256 to defeat rainbow tables.

⚠️ Breaking change: because the hash function changed (salted vs. unsalted SHA256), existing accounts cannot log in after upgrade and must be re-created. Clear the auth table before deploying, or coordinate a migration.

6. Stored XSS in admin dashboard (Moderate)

admin.html rendered record.owner and vector_clock keys with innerHTML. Any user who PUT a record with an owner containing &lt;img src=x onerror=...&gt; could pop JS in the admin's browser the next time they viewed the record.

Fix: replaced the template-literal innerHTML block with safe DOM construction (textContent + createElement + appendChild).

7. Committed weak credentials (Moderate)

.env was tracked in the repo with JWT_SECRET=123, ADMIN_PASSWORD=admin123, CLUSTER_SECRET=please_change.

Fix: removed .env, added it to .gitignore, and added a .env.example with guidance. Anyone who has ever deployed using those values should rotate them immediately — they are now in the public git history.

Test plan

  • cargo build succeeds with no warnings.
  • Boot a node with no env file → process exits with a clear fatal: ... refusing to start message.
  • Boot a node with JWT_SECRET=123 → process exits (too short).
  • Boot a node with CLUSTER_SECRET=default_secret → process exits (forbidden value).
  • Boot two properly-configured nodes; verify replication still works and that internal URLs follow DYNA_MODE.
  • GET /auth/store without a token → 401.
  • GET /default/store with user A's token → only returns rows where owner == A.
  • PUT /..%2Fetc/key/foo → 400 Invalid table or key name, no file written outside ./data.
  • Register a user; replay the request concurrently → exactly one record, no inconsistent state.
  • Log in as admin; PUT a record with owner = "<img src=x onerror=alert(1)>"; reopen it in the dashboard → renders as text, no alert.

https://claude.ai/code/session_01NYckTQH6APLpfGKVwwcQVD


Generated by Claude Code

Unauthenticated data exfiltration
- GET /{table}/store, /{table}/keys, POST /{table}/keys and the SSE
  /{table}/subscribe/{key} endpoint required no authentication, so any
  caller could dump every record in a table — including the auth table
  with all users' hashed passwords. They now require a valid JWT and only
  return rows owned by the caller.

Insecure secret fallbacks
- ADMIN_PASSWORD silently defaulted to "admin123", CLUSTER_SECRET to
  "default_secret", and admin JWT signing to "default_jwt_secret". A
  misconfigured node therefore accepted X-Internal-Request: default_secret
  on the get_value / delete_value / put_value_internal endpoints, granting
  full read/write/delete to anyone on the network. main.rs now refuses to
  start unless JWT_SECRET, CLUSTER_SECRET and ADMIN_PASSWORD are set,
  non-empty, longer than 16 bytes and not in the known-placeholder list.
  admin_login no longer falls back to a hard-coded password and uses a
  constant-time comparison.

Path traversal via {table}/{key}
- Table and key names from URL paths flow into Path::join in
  persistance.rs. Names containing "/", "\\", "\\0", "." or ".." (or
  exceeding 255 bytes) are now rejected at every handler that touches the
  store before they can reach the persistence layer.

Plaintext cluster traffic when DYNA_MODE=https
- engine.rs / admin.rs / authentication.rs hard-coded http:// when
  building internal replication URLs, leaking the cluster secret and
  every replicated payload even when TLS was configured. All internal
  URL construction now goes through network::broadcaster::build_url so
  the scheme follows DYNA_MODE.

Race condition + ownership forgery in user registration
- The check-then-insert in authentication::access could race two
  concurrent registrations of the same username. The internal replication
  branch also trusted an attacker-controlled "User" header as the record
  owner. Registration is now atomic via DashMap's entry API and the owner
  is always the path-derived username. Passwords are salted with the
  username before hashing (note: existing accounts must re-register).

Stored XSS in admin dashboard
- admin.html rendered record.owner and vector_clock keys with innerHTML,
  so any value an attacker could PUT would execute JS in the admin's
  browser. Replaced with safe DOM construction (textContent).

Committed weak credentials
- Removed the tracked .env containing JWT_SECRET=123 / ADMIN_PASSWORD=
  admin123 / CLUSTER_SECRET=please_change, added .env to .gitignore, and
  shipped a .env.example with guidance.

https://claude.ai/code/session_01NYckTQH6APLpfGKVwwcQVD
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants